Skip to content

Fix: Avoid following PURL redirects in unit tests#43

Merged
alexskr merged 3 commits intomasterfrom
fix/purl-test-no-ui-redirect
Jul 29, 2025
Merged

Fix: Avoid following PURL redirects in unit tests#43
alexskr merged 3 commits intomasterfrom
fix/purl-test-no-ui-redirect

Conversation

@alexskr
Copy link
Member

@alexskr alexskr commented Jul 28, 2025

Updated PURL-related tests to assert only the HTTP 302 status and Location header, without following the redirect to the BioPortal UI. This avoids triggering Cloudflare bot protection during CI runs.

Resolves:

Updated PURL-related tests to assert only the HTTP 302 status and Location header,
without following the redirect to the BioPortal UI. This avoids triggering
Cloudflare bot protection during CI runs.

We do not want to test UI behavior here — we only verify that the
correct redirect is served by the PURL resolver. Testing how the UI handles or
rewrites URLs (e.g., with query parameters or concept ID fragments) is out of scope
for these tests.

Split PURL resolution tests into a new file test_class_purl.rb to separate
concerns from core class model tests in test_class.rb

Resolves /issues/41
@alexskr alexskr marked this pull request as ready for review July 28, 2025 23:43
@alexskr alexskr requested review from jvendetti and mdorf July 28, 2025 23:43
@jvendetti
Copy link
Member

We do not want to test UI behavior here — we only verify that the
correct redirect is served by the PURL resolver. Testing how the UI handles or
rewrites URLs (e.g., with query parameters or concept ID fragments) is out of scope
for these tests.

I'm not sure I'm following this comment. These unit tests weren't added to test UI behavior. They were added because the purl method in this library was (in some cases) generating invalid PURLs that resulted in 404 errors. See this comment for more details. The fix had to be applied to the code in this library, hence the new unit tests.

@alexskr
Copy link
Member Author

alexskr commented Jul 28, 2025

We do not want to test UI behavior here — we only verify that the
correct redirect is served by the PURL resolver. Testing how the UI handles or
rewrites URLs (e.g., with query parameters or concept ID fragments) is out of scope
for these tests.

I'm not sure I'm following this comment. These unit tests weren't added to test UI behavior. They were added because the purl method in this library was (in some cases) generating invalid PURLs that resulted in 404 errors. See this comment for more details. The fix had to be applied to the code in this library, hence the new unit tests.

You're right — the test likely wasn't originally intended to validate UI behavior. But in practice, it was doing exactly that.

Here’s what was happening:

The test called http://purl.bioontology.org/ontology/SNOMEDCT/64572001

The PURL server responded with a 302 redirect to
https://bioportal.bioontology.org/ontologies/SNOMEDCT/classes/64572001

The test followed that redirect to the UI, and the UI itself issued another redirect to
https://bioportal.bioontology.org/ontologies/SNOMEDCT?p=classes&conceptid=64572001

The test then asserted that the final URL matched that rewritten form — which is handled by the frontend.
So while the test may have been added to catch issues with purl generation, it ended up validating how the UI rewrites and serves the redirected URL, not just the correctness of the purl itself.

Copy link
Member

@mdorf mdorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes themselves appear sound.

@jvendetti
Copy link
Member

Yes -- I'm familiar with what's happening with the redirect chain, which is why I added the faraday-follow_redirects gem. Following the redirect chain allowed me to be certain that the system was functioning end-to-end and that the PURLs generated by this library didn't ultimately result in 404s in the Rails application.

@jvendetti
Copy link
Member

Split PURL resolution tests into a new file test_class_purl.rb to separate concerns from core class model tests in test_class.rb

I disagree with moving these tests to a separate file. It's a best practice to have one test file per class/module. The tests are exercising the purl method, which is a member of Class. I don't see a reason to fragment the unit tests for Class, particularly when the file is less than 100 lines.

@alexskr
Copy link
Member Author

alexskr commented Jul 29, 2025

Yes -- I'm familiar with what's happening with the redirect chain, which is why I added the faraday-follow_redirects gem. Following the redirect chain allowed me to be certain that the system was functioning end-to-end and that the PURLs generated by this library didn't ultimately result in 404s in the Rails application.

Validating the full redirect chain is sensible when the goal is to ensure that a PURL ultimately lands on a functioning UI page.

That said, I think this kind of end-to-end check might be better suited for an integration test on the UI side, rather than in the Ruby client’s unit test suite. My understanding is that from the perspective of this library, its responsibility is to retrieve a valid .purl from the API and ensure that it resolves; it is not responsible for testing how the UI handles or rewrites that redirect once it is received.

By limiting the test to just verifying the initial redirect from the PURL server (i.e., checking the Location header and status), we still catch broken or malformed PURLs without relying on frontend behavior or risking Cloudflare-related CI failures.

@alexskr
Copy link
Member Author

alexskr commented Jul 29, 2025

Split PURL resolution tests into a new file test_class_purl.rb to separate concerns from core class model tests in test_class.rb

I disagree with moving these tests to a separate file. It's a best practice to have one test file per class/module. The tests are exercising the purl method, which is a member of Class. I don't see a reason to fragment the unit tests for Class, particularly when the file is less than 100 lines.

Totally fair — and I agree with the principle that test files generally map to class/module boundaries. Since purl is a method on Class, it's reasonable to keep tests in test_class.rb.

That said, my thinking here is based more on functional separation than method ownership. The purl tests — at least in their current form — are really validating the redirect behavior of the PURL service, not just how the client constructs .purl, it goes a step further by making a live request to the PURL and asserting that the redirect target is correct.

Given that, those tests felt conceptually separate from the core model tests, which focus on how the client loads and structures data returned from the API. Since this library’s primary role is to wrap API responses into objects for downstream use (e.g., in the UI), checking how an external redirect behaves feels like a different concern.

I think one way to do this more cleanly would be to move the simple .purl retrieval checks back into test_class.rb, since that’s part of what the class is directly responsible for. Then we could move the redirect resolution tests (i.e., the live HTTP calls and 302 checks) into a dedicated test file. That would keep the test suite aligned with both class boundaries and functional responsibilities, while also making it easier to skip or isolate external network tests for users who don’t rely on PURLs.

That’s the reasoning anyway; I’m not strongly opposed to keeping everything in one file if that’s the preferred approach. Just wanted to lay out the rationale in case it’s useful.

alexskr added 2 commits July 28, 2025 18:31
… UI redirects

Updated tests for the `.purl` method to:
- Assert the constructed URL matches the expected output based on client config
- Confirm the PURL resolves via HTTP 302 to the correct UI target
- Avoid following the UI-level redirect to prevent Cloudflare issues in CI
- Used `@@purl_prefix` from config.purl_prefix.
@alexskr
Copy link
Member Author

alexskr commented Jul 29, 2025

@jvendetti, I reverted the unit test file split; it's easier that way.

@alexskr alexskr changed the title Fix: Avoid following PURL redirects and split PURL tests into a separate file Fix: Avoid following PURL redirects in unit tests Jul 29, 2025
@alexskr alexskr merged commit 22df30d into master Jul 29, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants